Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove this-> from decltype #3723

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Remove this-> from decltype #3723

merged 2 commits into from
Nov 22, 2023

Conversation

PeterJohnson
Copy link
Contributor

The latest version of MSVC doesn't like the use of this-> in decltype, and removing it from the decltype doesn't seem to harm anything. It's unclear whether this is a bug in MSVC, or a standards-compliant change, given that other compilers don't reject either variant. It feels like this might be a bug in the compiler, as the error messages make it look like MSVC is trying to look up the function in the class where it's being used, rather than the class where it's being defined (e.g. the this pointer maybe isn't properly tracked/associated)?

An identical patch (wpilibsuite/allwpilib#5948) fixes the build failures we are seeing with current HEAD.

Rough code (full code is https://github.com/calcmogul/allwpilib/blob/6e23e70a8c48266ca25fbb818ce8357dce80ac32/wpilibc/src/main/native/cpp/Preferences.cpp):

std::string_view GetPath();
namespace { struct Instance {
std::string path = fmt::format("{}/", GetPath());
}; }

On MSVC 19.38.33129 or similar (e.g. using the latest GitHub Windows-2022 image), this errors with the following (full build log at https://github.com/wpilibsuite/allwpilib/actions/runs/6937548373/job/18871818708?pr=5945):

C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(1438): error C2039: 'map': is not a member of '`anonymous-namespace'::Instance'
C:\work\wpilibc\src\main\native\cpp\Preferences.cpp(23): note: see declaration of '`anonymous-namespace'::Instance'
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(1438): note: the template instantiation context (the oldest one first) is
C:\work\wpilibc\src\main\native\cpp\Preferences.cpp(33): note: see reference to function template instantiation 'fmt::v10::basic_format_string<char,std::basic_string_view<char,std::char_traits<char>>>::basic_format_string<char[4],0>(const S (&))' being compiled
        with
        [
            S=char [4]
        ]
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(2740): note: see reference to class template instantiation 'fmt::v10::detail::format_string_checker<Char,std::basic_string_view<char,std::char_traits<char>>>' being compiled
        with
        [
            Char=char
        ]
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(2608): note: while compiling class template member function 'fmt::v10::detail::format_string_checker<Char,std::basic_string_view<char,std::char_traits<char>>>::format_string_checker(fmt::v10::basic_string_view<Char>)'
        with
        [
            Char=char
        ]
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(2609): note: see reference to alias template instantiation 'fmt::v10::detail::mapped_type_constant<std::basic_string_view<char,std::char_traits<char>>,fmt::v10::basic_format_context<fmt::v10::appender,char>>' being compiled
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(1478): note: see reference to class template instantiation 'fmt::v10::detail::arg_mapper<fmt::v10::basic_format_context<fmt::v10::appender,char>>' being compiled
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(1462): error C2039: 'do_map': is not a member of '`anonymous-namespace'::Instance'
C:\work\wpilibc\src\main\native\cpp\Preferences.cpp(23): note: see declaration of '`anonymous-namespace'::Instance'
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(1468): error C2039: 'map': is not a member of '`anonymous-namespace'::Instance'
C:\work\wpilibc\src\main\native\cpp\Preferences.cpp(23): note: see declaration of '`anonymous-namespace'::Instance'
C:\work\wpiutil\src\main\native\thirdparty\fmtlib\include\fmt\core.h(2609): error C2062: type 'unknown-type' unexpected

@PeterJohnson
Remove this-> from decltype
8d2bebb 
The latest version of MSVC doesn't like it, and removing it doesn't seem to harm anything.
@PeterJohnson
Copy link
Contributor Author

Interesting... it fails with g++ 4.8, but appears to work with everything newer (although the builds were canceled so it's hard to tell for sure).

@vitaut
Copy link
Contributor

vitaut commented Nov 22, 2023

Looks like a compiler bug, please report to Microsoft.

Closing as earlier version of gcc require this->.

@vitaut vitaut closed this Nov 22, 2023
@PeterJohnson
Copy link
Contributor Author

I’ll report the bug, but since it’s in a released version of the compiler that’s active in the Windows-2022 GitHub image, I wouldn’t be surprised if other people started running into this with vcpkg in the near term, so it might be worth an ifdef’ed version of this as a workaround if other people start reporting issues.

@vitaut
Copy link
Contributor

vitaut commented Nov 22, 2023 •
edited
Loading

It's unfortunate but we cannot break gcc and this particular case seems to only affect default member initializers in MSVC which can be worked around. Conditionally disabling this will likely cause more problems.

@PeterJohnson
Copy link
Contributor Author

PeterJohnson commented Nov 22, 2023 •
edited
Loading

Given it works without this-> in all more recent compilers (MSVC, newer GCC, and clang), and the body of the function doesn't use this->, isn't the current use of this-> in the decltype actually a workaround for a bug in GCC 4.8?

@vitaut
Copy link
Contributor

vitaut commented Nov 22, 2023

Possibly but more likely it's a newer feature (either in the standard or in the implementation). The core of {fmt} only requires a subset of C++11 supported by older compilers including gcc 4.x.

@phprus
Copy link
Contributor

phprus commented Nov 22, 2023

@vitaut

GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57543
Fixed in GCC 5.

I think we can add macro:

if GCC < 5
#define FMT_DETAIL_DECLTYPE_THIS  this->
else
#define FMT_DETAIL_DECLTYPE_THIS
endif

and use it in decltype.

@vitaut
Copy link
Contributor

vitaut commented Nov 22, 2023

That'll work (although I'd drop "DETAIL_" from the macro name). Not sure why I thought ifdef would be a problem =).

@phprus
Copy link
Contributor

phprus commented Nov 22, 2023

@vitaut
Godbolt: https://godbolt.org/z/M1P8Yessq

@PeterJohnson
Copy link
Contributor Author

PeterJohnson commented Nov 22, 2023 •
edited
Loading

I pushed the ifdef version to my branch for this PR, https://github.com/PeterJohnson/fmt/tree/remove-decltype-this (I guess this PR won't actually pick it up unless it's reopened?)

@vitaut vitaut reopened this Nov 22, 2023
@vitaut
Copy link
Contributor

vitaut commented Nov 22, 2023

Reopened

@vitaut vitaut merged commit dd6f657 into fmtlib:master Nov 22, 2023
80 checks passed
@vitaut
Copy link
Contributor

vitaut commented Nov 22, 2023

Merged, thanks.

@PeterJohnson PeterJohnson deleted the remove-decltype-this branch November 22, 2023 17:48
@PeterJohnson
Copy link
Contributor Author

Thanks! For completeness, I've reported this bug to Microsoft here: https://developercommunity.visualstudio.com/t/decltypethis-member-fails-with-C203/10523643

@BrewTestBot BrewTestBot mentioned this pull request Jan 1, 2024
Merged
happymonkey1 pushed a commit to happymonkey1/fmt that referenced this pull request Apr 7, 2024
@PeterJohnson @happymonkey1
Remove this-> from decltype (fmtlib#3723)
c09ffd8 
* Remove this-> from decltype

The latest version of MSVC doesn't like it, and removing it doesn't seem to harm anything.

* Add ifdef for GCC < 5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PeterJohnson @vitaut @phprus